Claude finished @Yeraze's task —— View job

PR Review: feat(map-analysis): 3D terrain view + DEM tile proxy (#3826 Phase 2)

This is a well-structured, thoughtfully-engineered PR. The code is clean, the security model is sound, and the test coverage is thorough. Below are my findings, organized by severity.

Security

No issues found. The sensitive areas were handled correctly:

• The elevation source URL and API key are never echoed in responses, logs, or the capabilities envelope (tested at elevationRoutes.test.ts:226,276).
• All outbound HTTP goes through safeFetch/SSRF guard — no raw fetch in the proxy path.
• Tile coordinate validation (parseTileParam + isValidTileCoord) rejects extensions like 123.png, non-integers, negative coords, and zoom beyond the terrarium max (15) before any URL substitution occurs.
• The tile proxy uses optionalAuth (not public-anonymous), consistent with the /profile endpoint.

Potential Bugs / Issues

1. getAllSettings() called once per tile request — no settings cache
src/server/routes/elevationRoutes.ts:161

const settings = await databaseService.settings.getAllSettings();

The tile proxy fetches all settings on every request. A single 3D map view fires dozens of tile requests concurrently; at 600 req/min that's 600 settings DB reads/min. The existing /profile route does the same, but it's rate-limited at 20/min. Since settings rarely change, caching them for a few seconds (or using a dedicated getSettingAsync('elevationEnabled') + getSettingAsync('elevationSourceUrl') pair) would substantially reduce DB pressure. This isn't a correctness bug, but worth noting for high-traffic deployments.

2. rawTileCache is module-scoped and shared across tests — can cause order-dependent test failures
src/server/services/elevationProvider.ts:157

const rawTileCache = new LruCache<string, Buffer>(RAW_TILE_CACHE_MAX);

The SSRF-blocked test in elevationRoutes.test.ts:297 already works around this by using a distinct tile coordinate (8/41/97 instead of 8/40/96). This is a valid mitigation, but the pattern is fragile — a future test that inadvertently reuses a coordinate warmed by a previous test will get a cache hit and bypass mockSafeFetch. A clearForTesting() export or resetting the module between tests would make this more robust. The same concern applies to tileCache and jsonCache, though those existed before this PR.

3. Basemap change removes the layer before the source — order matters if MapLibre defers
src/components/map/Base3DMap.tsx:275-276

if (map.getLayer(BASEMAP_LAYER_ID)) map.removeLayer(BASEMAP_LAYER_ID);
if (map.getSource(BASEMAP_SOURCE_ID)) map.removeSource(BASEMAP_SOURCE_ID);

This is the correct order (remove layer, then source). Just confirming it's right — MapLibre will throw if you try to remove a source that still has layers referencing it. ✓

4. basemap3d.ts:99 — unknown tileset falls back to OSM without setting usedFallback: true
src/config/basemap3d.ts:98-102

it('falls back to the default tileset (osm) for an unknown tileset id', () => {
    const result = resolve3DBasemap('does-not-exist', []);
    expect(result.usedFallback).toBe(false);  // ← test asserts usedFallback:false

If a user has a tileset ID that's no longer valid (e.g., removed from TILESETS), getTilesetById silently returns the OSM fallback but usedFallback is set to false. The on-map note ("Showing default basemap in 3D — the selected map style is vector-only") won't appear, even though the user's tileset isn't being used. Depending on getTilesetById's contract, it may be intentional, but it's worth noting.

Performance Considerations

5. DEM tile proxy performs a settings read on every tile fetch
(See item 1 above — repeating here under performance since it's the bigger concern.)

6. toNodesFeatureCollection is called on every nodes or loaded change
src/components/map/Base3DMap.tsx:293-297
This is called both in the nodes sync effect and at map construction time (via the map.on('load', ...) closure). For large node sets this is a minor concern, but the function isn't memoized. A useMemo wrapping toNodesFeatureCollection(nodes) at the component level would avoid the double computation when loaded flips true while nodes is unchanged (effect runs twice on that render cycle — once from the initial run since loaded just changed, once since nodes didn't). Low priority.

Code Quality / Best Practices

7. console.warn in Base3DMap.tsx
src/components/map/Base3DMap.tsx:181

console.warn('Base3DMap: WebGL map construction failed', err);

The rest of the codebase uses the project logger (src/utils/logger.js) for structured logging. console.warn is fine here since this is a frontend component (logger is backend), and this matches what MapLibre itself does — no change needed. Just noting it's intentional.

8. The useMemo(() => buildTerrainTileUrl(appBasename), []) empty-deps array
src/components/MapAnalysis/MapAnalysisCanvas.tsx:135

const terrainTileUrl = useMemo(() => buildTerrainTileUrl(appBasename), []);

appBasename is a module-scope constant, so this is technically correct and the eslint react-hooks/exhaustive-deps exception in the mount effect (// eslint-disable-next-line) handles the more complex case. However, useMemo with [] is equivalent to a useRef initialized once — consider making it a module constant instead since appBasename never changes:

const TERRAIN_TILE_URL = buildTerrainTileUrl(appBasename);

This is stylistic — the current code is functionally correct.

9. MapLibre maxzoom: 15 hardcoded in Base3DMap.tsx
src/components/map/Base3DMap.tsx:200

maxzoom: 15,

MAX_TERRARIUM_TILE_ZOOM is already exported from elevationProvider.ts (= 15). Importing and using it here would keep the two in sync if the constant ever changes, though 15 is the AWS terrarium dataset's fixed native zoom, so in practice this won't drift. Minor.

Test Coverage

Coverage is excellent:

• Route-level integration tests via createRouteTestApp harness ✓ (correct per CLAUDE.md)
• Unit tests for all service/provider functions ✓
• Component tests with a full FakeMap that tracks all MapLibre API calls ✓
• WebGL-unavailable paths tested (probe fail + constructor throw + StrictMode double-mount) ✓
• Secret/key leak assertions in both capabilities and tile error responses ✓
• Cache ordering issue documented and worked around with distinct tile coords ✓

One gap: There's no test for the basemap-change effect (removing and re-adding the raster source when basemap prop changes). The node-update path is tested in Base3DMap.test.tsx:248, but the basemap swap isn't. Low priority since the MapLibre API calls (removeLayer/removeSource/addSource/addLayer) are already on the FakeMap, making such a test easy to add.

Summary

This is a high-quality implementation. The security-sensitive paths (URL/key isolation, SSRF guard, coordinate validation) are all handled correctly and tested. The two items most worth addressing before merge are:

1. Settings read per tile — consider caching or using targeted getSetting calls to reduce DB pressure under the 600/min tile rate limiter.
2. Module-scoped cache isolation in tests — the coordinate-dodge workaround works but is fragile; a clearForTesting() export would be safer.

Everything else is minor or stylistic. The 2D/Leaflet path is provably untouched by the feature flag logic, the force-2D guard prevents stale-persisted-3D from stranding users, and the WebGL fallback is solid.

Claude finished @Yeraze's task —— View job

PR Review: feat(map-analysis): 3D terrain view + DEM tile proxy (#3826 Phase 2)

Tasks

• Gather context and understand the PR
• Review backend changes (elevationRoutes, elevationProvider, elevationService)
• Review frontend changes (Base3DMap, MapAnalysisCanvas, basemap3d)
• Review hooks and test coverage
• Post final review

This is a well-engineered PR. The security model is sound, the architecture is clean, and the test coverage is thorough. Below are my findings organized by severity.

Security — No Issues

The sensitive areas are all handled correctly:

• elevationSourceUrl and API keys are never echoed in responses, logs, or the capabilities envelope — verified by secret-leak assertions in both the capabilities and tile-error test cases.
• All outbound HTTP goes through safeFetch/SSRF guard. No raw fetch in the proxy path.
• parseTileParam + isValidTileCoord reject extensions like 123.png, non-integers, negatives, and zoom beyond the terrarium max (15) before any URL substitution.
• The tile proxy is optionalAuth (not fully public), consistent with the /profile endpoint.

Bugs / Issues

1. The profile route still uses getAllSettings() while the tile route now uses readElevationSettings() — inconsistency
src/server/routes/elevationRoutes.ts:62

The PR correctly introduced readElevationSettings() (two targeted getSetting calls) to reduce DB pressure on the tile proxy, and the review in the previous comment identified this as a concern for the tile route. But the fix was applied only to the tile route — POST /profile (line 62) still calls getAllSettings(). The two routes should use the same helper for consistency:

// POST /profile (line 62) — still uses getAllSettings()
const settings = await databaseService.settings.getAllSettings();

While /profile is rate-limited at 20/min vs. 600/min for tiles, the inconsistency is worth noting and the fix is trivial.

2. Unknown tileset ID falls back to OSM but usedFallback is false — silent substitution
src/config/basemap3d.ts:45-63 and src/config/basemap3d.test.ts:98-102

When getTilesetById receives an unknown ID (not a predefined ID, not in customTilesets), it silently returns TILESETS[DEFAULT_TILESET_ID] (OSM). In resolve3DBasemap, the tileset.isVector check fails to trigger for OSM (since OSM is a raster tileset), so usedFallback is set to false. Result: a user with a stale/deleted tileset ID silently gets OSM in 3D with no on-map note.

The test at line 98-102 actually documents this as expected behavior (usedFallback: false), but it may not be the intended UX. Worth a deliberate decision: should getTilesetById returning the default fallback trigger usedFallback: true? The contract would need to be documented if the current behavior is intentional.

3. INVALID_TILE_COORDS validation occurs after TERRAIN_TILES_UNAVAILABLE — means a JSON source with invalid coords returns 409, not 400
src/server/services/elevationService.ts:261-284

The validation order in fetchTerrainTile checks caps.provider !== 'terrarium' (→ 409) before isValidTileCoord (→ 400). A request to /tiles/999/0/0 with a JSON source URL returns 409 rather than 400. This is debatable — the spec says "validation order: first failure wins" — but it means coordinate validation can be bypassed by observing the 409 vs. 400 response code, revealing information about the server's elevation source type even when the request has obviously invalid coords. Very minor, but worth noting for defense-in-depth.

Performance

4. rawTileCache is module-scoped and cleared in beforeEach by the new clearRawTileCacheForTesting() export

The prior review noted the cache isolation concern; the export clearRawTileCacheForTesting (src/server/services/elevationProvider.ts:165-167) now exists and is used in beforeEach (elevationRoutes.test.ts:80). This fully resolves the earlier fragility concern. ✓

5. toNodesFeatureCollection is called twice per render when loaded flips to true
src/components/map/Base3DMap.tsx:292-297

The nodes effect deps include [nodes, loaded]. When loaded becomes true, this effect fires and calls setData with the same nodes that were already passed to addSource (line 212) in the map.on('load', ...) handler that just set loaded = true. So the GeoJSON source gets its data set twice on initial load. This is harmless (a redundant setData) but worth noting. A simple guard:

// Inside the nodes effect:
if (!map || !loaded) return;
const source = map.getSource(NODES_SOURCE_ID) as maplibregl.GeoJSONSource | undefined;
// Only need to call setData for *changes* after initial load —
// the load handler already supplied the initial data.
source?.setData(toNodesFeatureCollection(nodes));

The current code is functionally correct; this is a minor optimization.

Code Quality / Best Practices

6. maxzoom: 15 hardcoded in Base3DMap.tsx — use the exported constant
src/components/map/Base3DMap.tsx:200

MAX_TERRARIUM_TILE_ZOOM is already exported from elevationProvider.ts (line 61). Using it here would keep the two values in sync:

// src/components/map/Base3DMap.tsx line 200
maxzoom: MAX_TERRARIUM_TILE_ZOOM,

The literal 15 is the AWS terrarium dataset's fixed max zoom so it won't drift in practice, but the constant is already there.

7. useMemo(() => buildTerrainTileUrl(appBasename), []) — equivalent to a module constant
src/components/MapAnalysis/MapAnalysisCanvas.tsx:135

appBasename is a module-scope constant, so useMemo with [] is functionally equivalent to:

const TERRAIN_TILE_URL = buildTerrainTileUrl(appBasename); // module-level

Either way is correct. The useMemo is slightly more idiomatic for values that depend on module-scope imports in React components, but if appBasename were somehow unstable, the [] dep array would be a bug. Since it's a constant, making it a module-level constant is more explicit.

8. AttributionControl double-lists basemap attribution
src/components/map/Base3DMap.tsx:188-193

map.addControl(
  new maplibregl.AttributionControl({
    compact: true,
    customAttribution: [basemap.attribution, ELEVATION_ATTRIBUTION],
  }),

The basemap source already has an attribution field in the style definition (line 162: attribution: basemap.attribution). MapLibre automatically reads source.attribution and shows it in the attribution control. Passing it again in customAttribution may cause it to appear twice in the attribution popup. Worth verifying in the live demo.

Test Coverage

Coverage is excellent overall:

• Route-level integration tests via createRouteTestApp harness ✓ (correct per CLAUDE.md)
• Unit tests for service/provider functions ✓
• Base3DMap tests with a full FakeMap tracking all MapLibre API calls ✓
• WebGL-unavailable paths (probe fail + constructor throw + StrictMode double-mount) ✓
• Secret/key leak assertions on both capabilities and tile error responses ✓
• clearRawTileCacheForTesting() now used in beforeEach — cache isolation is solid ✓

One gap: no test for the basemap-change effect
src/components/map/Base3DMap.tsx:271-289

The basemap-swap path (removeLayer → removeSource → addSource → addLayer) isn't exercised by any test in Base3DMap.test.tsx. The FakeMap already tracks all these calls (removeLayer, removeSource, addSource, addLayer), so adding a rerender test with a different basemap prop would be straightforward and would confirm the correct layer/source insertion order.

Summary

Must address before merge:

• None — no blocking bugs found.

Worth addressing:

1. Item 8: Double-attribution in AttributionControl — verify in the live demo that basemap attribution doesn't appear twice.
2. Item 2: Clarify/document the intended behavior when an unknown tileset ID is passed to resolve3DBasemap — is usedFallback: false correct there?

Nice to have:

• Item 1: Bring POST /profile in line with the tile route by using readElevationSettings() instead of getAllSettings().
• Item 6: Use MAX_TERRARIUM_TILE_ZOOM constant instead of hardcoded 15 in Base3DMap.tsx.
• Item 9: Add a basemap-change effect test.

The 2D/Leaflet path is provably untouched, the force-2D guard prevents stale-persisted-3D from stranding users, the WebGL fallback is solid, and the security-sensitive paths (URL/key isolation, SSRF guard, coordinate validation) are all correctly handled and tested.